Pinging @elastic/es-storage-engine (Team:StorageEngine)

Hi @burqen, I've created a changelog YAML for you.

I'm afraid that we should revert the change around not extending DeduplicateFieldInfosCodec in the default codecs. The reason is that Lucene would use SPI to load the Codec and it will just instantiate the codec with the no-args constructor and thus we won't get to deduplicate the fields. This only applies when a node is restarted for example and we need to read the codec from the SegmentInfo and I guess that it applies to search nodes in serverless too.

I can see that Lucene is used to load the codecs here, but they will all be wrapped afterwards. Is there some other place as well where the codecs are service loaded outside of CodecService? https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/index/codec/CodecService.java#L66-L69

Is there some other place as well where the codecs are service loaded outside of CodecService?

Yes, in Lucene, when a commit is read from disk (see https://github.com/apache/lucene/blob/e02bdb4c3c547488342b423e1b9b2b25519bd427/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java#L409-L412). We kind of rely implicitly that for reading a segment with a particular codec we can use the SPI loaded codec.

Is there some other place as well where the codecs are service loaded outside of CodecService?

Yes, in Lucene, when a commit is read from disk (see https://github.com/apache/lucene/blob/e02bdb4c3c547488342b423e1b9b2b25519bd427/lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java#L409-L412). We kind of rely implicitly that for reading a segment with a particular codec we can use the SPI loaded codec.

Got it! Thanks for pointing that out. I'll revert and find some other approach.

Good observation @fcofdez, this indeed makes wrapping approach not effective. But it should still be possible to loop once over field info in TSDBSyntheticIdCodec if we extend from DeduplicateFieldInfosCodec?

@martijnvg The problem is that the Elasticsearch*Codecs need to inherit from DeduplicateFieldInfosCodec (if we want the service loaded codecs to benefit from deduplication which I assume we do). Even if TSDBSyntheticIdCodec also inherit from DeduplicateFieldInfosCodec we would still need it to wrap the original Elasticsearch*Codec which is also a DeduplicateFieldInfosCodec. So we would still iterate over FieldInfos twice (and even do the deduplication work twice).

I tried a more brute force approach

1. Inline the synthetic id functionality in DeduplicatingFieldInfosFormat, protected behind useTsdbSyntheticId flag

    @Override
    public FieldInfos read(Directory directory, SegmentInfo segmentInfo, String segmentSuffix, IOContext iocontext) throws IOException {
        final FieldInfos fieldInfos = delegate.read(directory, segmentInfo, segmentSuffix, iocontext);
        if (useTsdbSyntheticId) {
            TSDBSyntheticIdCodec.ensureSyntheticIdFields(fieldInfos);
        }
        final FieldInfo[] deduplicated = new FieldInfo[fieldInfos.size()];
        int i = 0;
        for (FieldInfo fi : fieldInfos) {
            // ... The deduplication stuff
            if (useTsdbSyntheticId) {
                fieldInfo = TSDBSyntheticIdCodec.wrapFieldInfoOnRead(fieldInfo);
            }
            deduplicated[i++] = fieldInfo;
        }
        return new FieldInfosWithUsages(deduplicated);
    }

2. Set the flag from CodecService by propagating through the codec

if (useTsdbSyntheticId) {
    ((DeduplicateFieldInfosCodec) codec).useTsdbSyntheticId();
    codec = new TSDBSyntheticIdCodec(codec.getName(), codec); // Still wrap for PostingsFormat, more on that later
}

3. Profit?
   ... no. Because this makes the useTsdbSyntheticId state part of the codecs which are service loaded in other places, such as IndexDiskUsageAnalyzer which becomes super confused when it suddenly looks like the _id field have Postings (because it reads the FieldInfo through DeduplicatingFieldInfosFormat which pretends the posting exist on the read path).

This makes me think it's a bad idea to make deduplicating codec aware of of synthetic id all together. But I cannot figure out a way to get rid of the extra loop in any other way. All my ideas center around in one way or the other inject functionality into DeduplicateFieldInfosCodec and more specifically into the FieldInfoFormat that it provides.

This makes me come back to your comment @fcofdez . How important is it that the service loaded codec benefit from the deduplication? They will still be loaded, only they will not be wrapped with the deduplication. How does that weigh against this optimization that we are trying to do here?

Do you have any ideas? @fcofdez , @tlrx , @martijnvg ?

About PostingsFormat:
The Elasticsearch*Codecs define their own PostingsFormat. I tried to wrap those from inside DeduplicateFieldInfosCodec based on the useTsdbSyntheticId, but that seemed both hacky and error prone so I stopped.